KAFKA-7006 - remove duplicate Scala ResourceNameType in preference to…#5152
Conversation
… Java ResourceNameType.
| @Test | ||
| public void testMatching() throws Exception { | ||
| assertTrue(ACL1.equals(ACL1)); | ||
| public void testMatching() { |
There was a problem hiding this comment.
Took the opportunity to fix the compiler warnings in this test file, as the PR was small...
…ill never have ResourceNameType.ANY or UNKNOWN.
|
cc @cmccabe |
|
test this please |
…ssages from newer clients to get to the right part of KafkaApis.java
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@big-andy-coates : Thanks for the patch. LGTM. A couple of minor comments below.
| * | ||
| * @param resource the resource path to which the acls belong. | ||
| * the supplied resource will have a specific resource name type, | ||
| * i.e. the resource name type will not be ``ResourceNameType.ANY`` or ``ResourceNameType.UNKNOWN``. |
There was a problem hiding this comment.
Is this true? I thought the current implementation allows you to pass in ResourceNameType.ANY to retrieve ACLs specified on either LITERAL or PREFIX?
There was a problem hiding this comment.
I believe this is true. SimpleAclAuthorizer expects a concrete Resource, not something which could match multiple resources. The code is here:
override def removeAcls(resource: Resource): Boolean = {
inWriteLock(lock) {
val result = zkClient.deleteResource(resource)
updateCache(resource, VersionedAcls(Set(), 0))
updateAclChangedFlag(resource)
result
}
}
The messiness here is that this constraint is not expressed in the type system. We use the same enum to describe the name types a concrete resource can have vs. a resource filter.
There was a problem hiding this comment.
That's right. The enum has ANY and UNKNOWN values, but the code won't allow them to be passed to the authorizer. I documented so that people implementing their own would know they aren't going to get these values. It's part of the downside to standardizing on the Java ResourceNameType.
|
|
||
| def getMatchingAcls(resourceType: ResourceType, resourceName: String): Set[Acl] = { | ||
| val filter = new ResourceFilter(resourceType.toJava, resourceName, JResourceNameType.ANY) | ||
| val filter = new ResourceFilter(resourceType.toJava, resourceName, ResourceNameType.ANY) |
There was a problem hiding this comment.
already removed in another PR...
| case Prefixed => "/kafka-prefixed-acl" | ||
| case ResourceNameType.LITERAL => "/kafka-acl" | ||
| case ResourceNameType.PREFIXED => "/kafka-prefixed-acl" | ||
| case _ => throw new IllegalArgumentException("Unknown name type:" + nameType) |
There was a problem hiding this comment.
Probably better to say "invalid name type" since rather than "Unknown name type", since the name type may not be UNKNOWN
There was a problem hiding this comment.
Done, (but in an up coming PR)
| case Prefixed => "/kafka-prefixed-acl-changes" | ||
| case ResourceNameType.LITERAL => "/kafka-acl-changes" | ||
| case ResourceNameType.PREFIXED => "/kafka-prefixed-acl-changes" | ||
| case _ => throw new IllegalArgumentException("Unknown name type:" + nameType) |
There was a problem hiding this comment.
Probably better to say "invalid name type" since rather than "Unknown name type", since the name type may not be UNKNOWN
There was a problem hiding this comment.
Removed in an upcoming PR.
|
+1. LGTM after the posted small changes are made |
|
@cmccabe @junrao - thanks for reviews! (Unrelated test failure...) This looks good to merge, (assuming you're happy that the minor points raised are addressed in other PRs?). However, I'd prefer my other PR, (#5160), to be merged first, if at all possible, as there will be conflicts and I'd rather resolve in this PR. |
|
test this please |
# Conflicts: # clients/src/main/java/org/apache/kafka/common/acl/AclBinding.java # core/src/main/scala/kafka/admin/AclCommand.scala # core/src/main/scala/kafka/security/SecurityUtils.scala # core/src/main/scala/kafka/security/auth/Resource.scala # core/src/main/scala/kafka/security/auth/SimpleAclAuthorizer.scala # core/src/main/scala/kafka/server/KafkaApis.scala # core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala
#5152) remove duplicate Scala ResourceNameType in preference to in preference to Java ResourceNameType. This is follow on work for KIP-290 and PR #5117, which saw the Scala ResourceNameType class introduced. I've added tests to ensure AclBindings can't be created with ResourceNameType.ANY or UNKNOWN. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
apache#5152) remove duplicate Scala ResourceNameType in preference to in preference to Java ResourceNameType. This is follow on work for KIP-290 and PR apache#5117, which saw the Scala ResourceNameType class introduced. I've added tests to ensure AclBindings can't be created with ResourceNameType.ANY or UNKNOWN. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
4 participants
remove duplicate Scala ResourceNameType in preference to in preference to Java ResourceNameType.
See KAFKA-7006.
This is follow on work for KIP-290 and PR #5117, which saw the Scala ResourceNameType class introduced.
I've added tests to ensure AclBindings can't be created with ResourceNameType.ANY or UNKNOWN.
Committer Checklist (excluded from commit message)
@junrao, @cmccabe - this is a follow on PR for KIP-290.